- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.8k
 
          unnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types
          #15460
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
  
    unnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types
  
  #15460
              Conversation
5477205    to
    dff9f73      
    Compare
  
    | && expr.span.ctxt() == ctxt | ||
| && let Some(reduced) = reduce_expression(cx, expr) | ||
| && let Some(reduced) = reduce_expression(cx, expr, &mut applicability) | ||
| && reduced.iter().all(|e| e.span.ctxt() == ctxt) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of off-topic: I was forced to use the more verbose Vec::new() syntax in all the tests because, when I used vec![], this check would prevent the lint from firing. That doesn't sound quite right to me?
unnecessary_operation: don't lint on composite types with fields of uncertain typesunnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types
              
          
                clippy_lints/src/no_effect.rs
              
                Outdated
          
        
      | && let Some(end) = range.end => | ||
| { | ||
| if [start, end].into_iter().any(|e| expr_type_is_certain(cx, e)) { | ||
| Some([start, end].into_iter().collect()) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Some([start, end].into_iter().collect()) | |
| Some(vec![start, end]) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also, two more high level questions here:
- 
Should this also recurse into
reduce_expression()with the start and end expressions of the range, to reduce it even further? I see this also isn't consistently done for some other kinds of expressions, like for array literals and struct expressions, so I'd also be fine with leaving it, but I'm not sure why it's done this way. - 
Why do we return
None/not reduce ranges at all if their type isn't certain, instead of only reducing the applicability like we do for structs in general? It still seems worth linting range expression statements that exist solely to make the inner expressions type-check. 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think reducing too much in one go can end up creating some pretty confusing suggestions.. Although maybe they could make sense for the target audience of this lint (who I'm not really sure who is, except for people that have made a typo in a quick reproducer -- in which case the complete reduction might be actively counter-productive for understanding what the lint wants from them)
 - I could totally make that change, yes. I guess it was just me too closely copying the structure of the regular 
Structarm -- there, the suggestion is deleted when the type has an explicitDropimpl 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented 2. for now
5198c64    to
    e783aea      
    Compare
  
    e783aea    to
    aaa0940      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
91c4c17    to
    d7dbefd      
    Compare
  
    | 
           @y21 friendly ping  | 
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
d7dbefd    to
    9e1cd08      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
9e1cd08    to
    6555bfb      
    Compare
  
    | 
           This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.  | 
    
fixes #15381
changelog: [
unnecessary_operation]: reduce applicability on exprs with uncertain types